Skip to content

Enable metrics#136

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Danil-Grigorev:enable-metrics
Jul 9, 2020
Merged

Enable metrics#136
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Danil-Grigorev:enable-metrics

Conversation

@Danil-Grigorev
Copy link

@Danil-Grigorev Danil-Grigorev commented Jun 4, 2020 •
edited
Loading

Working on https://issues.redhat.com/browse/OCPCLOUD-784

This PR is adding support for reporting following prometheus metrics and also starting controller-runtime metrics server to make these metrics available for prometheus servers:

  • mapi_instance_create_failed: Total count of "create" cloud api errors
  • mapi_instance_update_failed: Total count of "update" cloud api errors
  • mapi_instance_delete_failed: Total count of "delete" cloud api errors

Labels on these metrics (for Azure):

prometheus.Labels{
  "name": machine.Name,
  "namespace": machine.Namespace,
  "reason": error.Reason,
}

Depends on PR openshift/machine-api-operator#609 introducing metrics support

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2020
JoelSpeed
JoelSpeed reviewed Jun 4, 2020
View reviewed changes
go.mod Outdated
sigs.k8s.io/yaml v1.2.0
)

replace github.com/openshift/machine-api-operator => github.com/Danil-Grigorev/machine-api-operator v0.2.1-0.20200604073207-04068f243c6b
Copy link

@JoelSpeed JoelSpeed Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to be moved back to master openshift before merge
Hold until we can identify whether this is absolutely necessary
/hold

@Danil-Grigorev
Copy link
Author

Danil-Grigorev commented Jun 17, 2020

/hold Waiting on openshift/machine-api-operator#590

@Danil-Grigorev Danil-Grigorev mentioned this pull request Jun 17, 2020
Merged
@Danil-Grigorev
Copy link
Author

Danil-Grigorev commented Jun 18, 2020

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2020
@Danil-Grigorev
Copy link
Author

Danil-Grigorev commented Jun 18, 2020

/retest

@Danil-Grigorev Danil-Grigorev requested a review from JoelSpeed June 18, 2020 14:29
@JoelSpeed
Copy link

JoelSpeed commented Jun 18, 2020

@Danil-Grigorev Seems there's a conflict, could you fix that then I'll give another review

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 18, 2020
JoelSpeed
JoelSpeed reviewed Jun 22, 2020
View reviewed changes
Copy link

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
elmiko
elmiko approved these changes Jun 30, 2020
View reviewed changes
Copy link

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

enxebre
enxebre reviewed Jul 1, 2020
View reviewed changes
go.mod Outdated
replace (
sigs.k8s.io/cluster-api-provider-aws => github.com/openshift/cluster-api-provider-aws v0.2.1-0.20200618031251-e16dd65fdd85
sigs.k8s.io/cluster-api-provider-azure => github.com/openshift/cluster-api-provider-azure v0.1.0-alpha.3.0.20200618001858-af08a66b92de
sigs.k8s.io/cluster-api-provider-gcp => github.com/openshift/cluster-api-provider-gcp v0.0.1-0.20200617145800-5c21c4940146
Copy link
Member

@enxebre enxebre Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed at all? gcp repo is not even a fork.

@enxebre
Copy link
Member

enxebre commented Jul 1, 2020

why is this commit called "Add health checks"?

@enxebre
Copy link
Member

enxebre commented Jul 1, 2020

this needs rebase.

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 2, 2020
@Danil-Grigorev
Copy link
Author

Danil-Grigorev commented Jul 7, 2020

/retest

1 similar comment
@JoelSpeed
Copy link

JoelSpeed commented Jul 7, 2020

/retest

@Danil-Grigorev Danil-Grigorev requested a review from elmiko July 7, 2020 10:38
@Danil-Grigorev
Copy link
Author

Danil-Grigorev commented Jul 7, 2020

@enxebre @JoelSpeed @alexander-demichev I'd like to merge those today. Please approve and LGTM

elmiko
elmiko approved these changes Jul 7, 2020
View reviewed changes
Copy link

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020
@JoelSpeed
Copy link

JoelSpeed commented Jul 8, 2020

/approve
/retest

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 8, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020
@openshift-bot
Copy link

openshift-bot commented Jul 8, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link

openshift-bot commented Jul 8, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Jul 8, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Jul 8, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Jul 8, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit fa840d1 into openshift:master Jul 9, 2020
@alexander-demicev alexander-demicev mentioned this pull request Dec 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelgugino michaelgugino Awaiting requested review from michaelgugino

@enxebre enxebre Awaiting requested review from enxebre

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

1 more reviewer

@elmiko elmiko elmiko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@elmiko elmiko

@JoelSpeed JoelSpeed

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Danil-Grigorev @JoelSpeed @enxebre @openshift-ci-robot @openshift-bot @elmiko @openshift-merge-robot